Streamline source creation#6849
Conversation
Signed-off-by: Tomas Longo <tlongo@sternad.de>
|
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(GrpcServiceConfigurator.class); | ||
| private static final String PIPELINE_NAME_PLACEHOLDER = "${pipelineName}"; | ||
| private static final String REGEX_HEALTH = "regex:^/(?!health$).*$"; |
There was a problem hiding this comment.
REGEX_HEALTH is duplicated across all three OTel sources (trace, metrics, logs). Extract to a shared constant in a common location.
| private static final String PIPELINE_NAME_PLACEHOLDER = "${pipelineName}"; | ||
| private static final String REGEX_HEALTH = "regex:^/(?!health$).*$"; | ||
| private static final RetryInfoConfig DEFAULT_RETRY_INFO = | ||
| new RetryInfoConfig(Duration.ofMillis(100), Duration.ofMillis(2000)); |
There was a problem hiding this comment.
DEFAULT_RETRY_INFO is duplicated identically in both GrpcServiceConfigurator and HttpServiceConfigurator. Extract to shared constant or config default.
| */ | ||
|
|
||
| package org.opensearch.dataprepper.plugins.source.otellogs.grpc; | ||
|
|
There was a problem hiding this comment.
GrpcServiceConfigurator.java / HttpServiceConfigurator.java. These are nearly identical across otel-trace, otel-metrics, and otel-logs (3 copies each). This is the same code triplicated. Should live in a shared otel-source-common module or otel-proto-common.
| * compatible open source license. | ||
| * | ||
| */ | ||
|
|
There was a problem hiding this comment.
GrpcServiceConfigurator.java / HttpServiceConfigurator.java. These are nearly identical across otel-trace, otel-metrics, and otel-logs (3 copies each). This is the same code triplicated. Should live in a shared otel-source-common module or otel-proto-common.
There was a problem hiding this comment.
They rely on the different config classes though. They need to be taken into consideration.
There was a problem hiding this comment.
Agree the config classes differ, but they all expose the same methods (getPath, hasHealthCheck, getCompression, getAuthentication, getRetryInfo). An interface like OTelSourceConfig defining those common methods would let a single generic configurator work across all three sources without losing type safety.
KarstenSchnitter
left a comment
KarstenSchnitter
left a comment
There was a problem hiding this comment.
Thanks for providing this PR.
It seems, that all sources require GrpcServiceConfiguration and HttpServiceConfiguration classes, that do not contain any differentiated behaviour. They could be copies of each other, but they still differ in small ways. @TomasLongo are these differences intentional or is it possible to align them completely? If they can be aligned, is there a place to reuse the code and avoid duplicates? I understand, that they have a dependency on the different Config classes.
| sb.service(builtGrpcService, DecodingService.newDecorator()); | ||
| } | ||
|
|
||
| authenticationProvider.getHttpAuthenticationService().ifPresent(sb::decorator); |
There was a problem hiding this comment.
Why is this handled differently from the trace GrpcServiceConfigurator? In the trace case a check exists if authentication is even required: https://github.com/sternadsoftware/data-prepper-second/blob/ff99b3dc6d98e40758dea9d662ed71f92c46bf3d/data-prepper-plugins/otel-trace-source/src/main/java/org/opensearch/dataprepper/plugins/source/oteltrace/grpc/GrpcServiceConfigurator.java#L90-L98
| * compatible open source license. | ||
| * | ||
| */ | ||
|
|
There was a problem hiding this comment.
They rely on the different config classes though. They need to be taken into consideration.
KarstenSchnitter
left a comment
KarstenSchnitter
left a comment
There was a problem hiding this comment.
One more license header, please check the automated test results.
Signed-off-by: Tomas Longo <tlongo@sternad.de>
3 participants
Description
[Describe what this change achieves]
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.